Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[16.0][FIX] hr_timesheet_sheet: deal with time off line #712

Merged
merged 1 commit into from
Sep 16, 2024
Merged

[16.0][FIX] hr_timesheet_sheet: deal with time off line #712

merged 1 commit into from
Sep 16, 2024

Conversation

maisim
Copy link

@maisim maisim commented Sep 10, 2024

Fix #711

@maisim maisim changed the title [FIX] Fixes that you can't add a line to a timesheet that contains a time off line [FIX][16.0] Fixes that you can't add a line to a timesheet that contains a time off line Sep 10, 2024
MiquelRForgeFlow
MiquelRForgeFlow approved these changes Sep 10, 2024
View reviewed changes
Copy link
Contributor

@MiquelRForgeFlow MiquelRForgeFlow left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested 👍

Also, technically correct.

@alexey-pelykh
Copy link
Contributor

A test would be great

AndreuOForgeFlow
AndreuOForgeFlow approved these changes Sep 10, 2024
View reviewed changes
Copy link

@AndreuOForgeFlow AndreuOForgeFlow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Functional review 👍

@maisim
Copy link
Author

maisim commented Sep 10, 2024

A test would be great

What kind of test are you thinking of? @alexey-pelykh

@alexey-pelykh
Copy link
Contributor

@maisim a test that ensures the fix fixes the issue. And also, that way when I comment out the fix, it breaks - so that we don't rely on manual testing of this issue in future.

@maisim
Copy link
Author

maisim commented Sep 12, 2024

@maisim a test that ensures the fix fixes the issue. And also, that way when I comment out the fix, it breaks - so that we don't rely on manual testing of this issue in future.

We cannot install modules from tests:
https://github.com/odoo/odoo/blob/4c91a4ba280e4709d391419cc9f6b39432bdbe5f/odoo/addons/base/models/ir_module.py#L580-L586

Our choices:

  1. We can add project_timesheet_holidays to the requierements in the CI for timesheet, but I think it is not really a good idea...
  2. Run only this test in a specific environment, but I don't know if it is feasible ?

Can you tell me how you would proceed?

In any case, if it looks like it will take too long to set up, I would like us to merge this and see about testing in another PR

@MiquelRForgeFlow
Copy link
Contributor

MiquelRForgeFlow commented Sep 12, 2024 •
edited
Loading

In any case, if it looks like it will take too long to set up, I would like us to merge this and see about testing in another PR

My vote is on merge it too.

@maisim maisim mentioned this pull request Sep 12, 2024
Open
@OCA-git-bot
Copy link
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

@MiquelRForgeFlow
Copy link
Contributor

Can we merge this? :)

@pedrobaeza
Copy link
Member

Not with that commit message:

https://github.com/OCA/odoo-community.org/blob/master/website/Contribution/CONTRIBUTING.rst#commit-message

@maisim
[FIX] hr_timesheet_sheet: deal with time off line
659eab8 
Fixes inability to add lines to timesheets that containstime off lines,
managed from project_timesheet_holidays.

Fixes OCA#711
@maisim maisim force-pushed the 16.0_fix_unable_to_add_line_to_sheet branch from b060cab to 659eab8 Compare September 16, 2024 08:56
@maisim
Copy link
Author

maisim commented Sep 16, 2024

@pedrobaeza @MiquelRForgeFlow
Sorry, I hope it's okay now

@pedrobaeza pedrobaeza added this to the 16.0 milestone Sep 16, 2024
@pedrobaeza pedrobaeza changed the title [FIX][16.0] Fixes that you can't add a line to a timesheet that contains a time off line [16.0][FIX] hr_timesheet_sheet: deal with time off line Sep 16, 2024
@pedrobaeza pedrobaeza linked an issue Sep 16, 2024 that may be closed by this pull request
hr_timesheet_sheet: Unable to add new line on sheet if project_timesheet_holidays is installed #711
Closed
@pedrobaeza
Copy link
Member

/ocabot merge patch

@OCA-git-bot
Copy link
Contributor

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 16.0-ocabot-merge-pr-712-by-pedrobaeza-bump-patch, awaiting test results.

This was referenced Sep 16, 2024
Merged
Open
@OCA-git-bot OCA-git-bot merged commit 0191578 into OCA:16.0 Sep 16, 2024
9 checks passed
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at fff331b. Thanks a lot for contributing to OCA. ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AndreuOForgeFlow AndreuOForgeFlow AndreuOForgeFlow approved these changes

@MiquelRForgeFlow MiquelRForgeFlow MiquelRForgeFlow approved these changes

@alexey-pelykh alexey-pelykh alexey-pelykh approved these changes

Assignees
No one assigned
Labels
approved merged 🎉 ready to merge
Projects
None yet
Milestone
16.0
Development

Successfully merging this pull request may close these issues.

hr_timesheet_sheet: Unable to add new line on sheet if project_timesheet_holidays is installed
6 participants
@maisim @alexey-pelykh @MiquelRForgeFlow @OCA-git-bot @pedrobaeza @AndreuOForgeFlow